-
Notifications
You must be signed in to change notification settings - Fork 879
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[core/ssr] Add Node builds that don't explode #3156
Conversation
🦋 Changeset detectedLatest commit: 8367869 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📊 Tachometer Benchmark ResultsSummarynop-update
render
update
update-reflect
Resultslit-element-list
render
update
update-reflect
lit-html-kitchen-sink
render
update
nop-update
lit-html-repeat
render
update
lit-html-template-heavy
render
update
reactive-element-list
render
update
update-reflect
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exciting!
rollup-common.js
Outdated
@@ -255,6 +255,7 @@ export function litProdConfig({ | |||
packageName, | |||
outputDir = './', | |||
copyHtmlTests = true, | |||
nodeBuild = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: naming, maybe includeNodeBuild
? (so it doesn't sound like you're configuring the build to be for node specifically)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
if (NODE_MODE) { | ||
global.HTMLElement ??= class HTMLElement {} as unknown as typeof HTMLElement; | ||
global.customElements ??= { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All things equal, if a user didn't explicitly load the dom-shim, it'd be nice to keep as much as possible off the global. I think we could pull HTMLElement into a local variable (and shim it there) rather than put it on the global, right?
customElements
is a little harder; we could do the same in the decorator, and that would just leave JS users to needing to do customElements?.define(...)
in their own elements. But maybe it's worth keeping customElements
on the global to help smooth that over?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I think if this approach is good it's exactly because it 1) puts all the cost on the node import, so 2) can go to extra lengths to isolate any shims to just this module.
I think we should be able to do something like:
const HTMLElementShim = class HTMLElement { ... }
export class ReactiveElement extends (NODE ? HTMLElementShim : HTMLElement) {
Regarding window.customElements
, shouldn't that be shimmed, if at all, in the decorator definition module? And there we just need the decorator to not access it, yeah?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's sync on this offline. It seems like Al is moving towards not having a DOM shim separate from 'lit is loaded in Node'? If that' the case, we do want to populate the global in the Node import condition, yeah?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per meeting this morning, HTMLElement
is no longer globally shimmed, it is instead just declared inline. The reason we don't need HTMLElement
globally is that we're only concerned with users who are extending ReactiveElement
or LitElement
.
I have kept the global customElements
definitions so that it will be possible to import components that aren't defined using the decorator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pushed another change that shims HTMLElement
in a slightly more sneaky way. It now temporarily defines globalThis.HTMLElement
, and then removes it after ReactiveElement
is defined, instead of changing the extends clause at all.
The reason for this is that I found that the following code ... :
export abstract class ReactiveElement
extends (NODE_MODE && global.HTMLElement === undefined
? (class HTMLElement {} as unknown as typeof HTMLElement)
: HTMLElement)
... resulted in in this d.ts
output:
declare const ReactiveElement_base: {
new (): HTMLElement;
prototype: HTMLElement;
};
/**
* Base element class which manages element properties and attributes. When
* properties change, the `update` method is asynchronously called. This method
* should be supplied by subclassers to render updates as desired.
* @noInheritDoc
*/
export declare abstract class ReactiveElement extends ReactiveElement_base implements ReactiveControllerHost {
Which, for some reason, causes ts-transformers to fail with this error:
--··"../reactive-element/reactive-element.d.ts(181,55):·error·TS2749:·'ReactiveElement_base'·refers·to·a·value,·but·is·being·used·as·a·type·here.·Did·you·mean·'typeof·ReactiveElement_base'?\n",
I also have a suspicion that the ternary extends
could be upsetting to Closure compiler.
Open to other suggestions, but this approach of defining and then undefining it should be the least confusing to type systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of any better way. I tried playing around with it a bit but TS also did not like an exported class extending something that's module scoped.
import {LitElement, html} from 'lit-element'; | ||
import {customElement} from 'lit-element/decorators.js'; | ||
|
||
@customElement('my-element') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense to include a version that doesn't use the decorator as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
/** | ||
* A tiny module scoped polyfill for HTMLSlotElement.assignedElements. | ||
*/ | ||
const slotAssignedElements = | ||
window.HTMLSlotElement?.prototype.assignedElements != null | ||
global.HTMLSlotElement?.prototype.assignedElements != null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is HTMLSlotElement
going to be defined in Node? Is that in the dom-shim? And what about all the other element classes? Users probably won't use ?.
for these as is done here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, and the broader DOM shim doesn't define HTMLSlotElement
either. Somebody could be doing e.g. instanceof HTMLSlotElement
somewhere, but I think we don't need to worry about it for now.
import {customElement} from '@lit/reactive-element/decorators.js'; | ||
import {ReactiveElement} from '@lit/reactive-element'; | ||
|
||
@customElement('my-element') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a version that doesn't use the decorator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
if (NODE_MODE) { | ||
global.HTMLElement ??= class HTMLElement {} as unknown as typeof HTMLElement; | ||
global.customElements ??= { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's sync on this offline. It seems like Al is moving towards not having a DOM shim separate from 'lit is loaded in Node'? If that' the case, we do want to populate the global in the Node import condition, yeah?
Just added a fix to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed by testing pre-release that this does indeed create node builds that don't explode.
); | ||
let replaced = false; | ||
const newSource = fileSource.replace(versionVarRegex, () => { | ||
const newSource = fileSource.replace(versionVarRegex, (_, pre, post) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh very nice utilization of capture groups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Since we have to commit to this approach long-term, I'm mildly interested in how much code it would add if we just did the shimming in the normal codepath (and not have a separate node export condition and all the config file overhead that requires).
But since as you note the node export condition option will also give us a nice place to put the isSSR
flag, seems like that having a NODE_MODE will end up being useful for a number of reasons, so I think I'm good with this direction.
Below is a comparison of the sizes of the two main affected files. "Prod" is the minified browser build (
So, a little more substantial on the Also I believe this difference will increase even further, because I plan to extend the Node |
Cool thanks. The 90-100b on RE is definitely worth it, plus what you said about possibly expanding it a bit, so I think I'm sold. |
Background
Before this PR, importing Lit with Node causes errors such as
window is not defined
. The only way to load Lit was to first ensure that thedom-shim.js
module provided by the@lit-labs/ssr
package was loaded, which defineswindow
plus APIs likeHTMLElement
andcustomElements
.This meant that using Lit in frameworks such as Next did not work out-of-the-box even when only client-side rendering is needed.
This change
After this PR, Lit can be imported from Node without errors, and custom elements can be defined as no-ops.
This doesn't remove the need for loading the DOM shim shim when SSR is actually needed, but it does give us compatibility at least for client-side rendering in frameworks like Next, without the user needing to do anything special.
This works by adding a Node-specific Rollup build and a
node
export condition tolit-html
and@lit/reactive-element
. Neitherlit-element
norlit
need a Node build, because only the underlying 2 libraries currently need Node-specific behavior.This only defines
customElements
as new globals. It does not definewindow
,HTMLElement
, or any other APIs. This way, we won't affect the behavior of libraries that detect whether they are in Node vs the browser by checking for awindow
global.For testing, I've added a
node-imports.ts
file to each package, and added a newnode:test
script which executes that module with Node. This confirms that Node doesn't crash on import.Fixes #3079
Followup
In a follow up, I will propose moving the actual functional dom-shim implementations of
HTMLElement
andcustomElements
from@lit-labs/ssr
intoreactive-element
(in this PR they are just no-ops) so that ultimately the user never has to load the dom-shim manually.However, in order to do this, we will need to update the Lit SSR
ModuleLoader
to understand export conditions -- because currently it does not support them, so it will not find the Node build. We depend on theresolve
package to do module resolution, which unfortunately does not yet support export conditions, so this work will be a little bit non-trivial.Another followup will be to add an
isSsr
const or function. Rather than doing runtime detection, this can simply betrue
in the node build andfalse
in the browser build.Size
This actually slightly drops the size of the main
lit-html.js
andreactive-element.js
production build files: